Skip to content

Use DevCert for SQL Server#13131

Open
afscrome wants to merge 2 commits intomicrosoft:mainfrom
afscrome:sqltls
Open

Use DevCert for SQL Server#13131
afscrome wants to merge 2 commits intomicrosoft:mainfrom
afscrome:sqltls

Conversation

@afscrome
Copy link
Contributor

@afscrome afscrome commented Nov 22, 2025

Description

This Pr updates the SQL Server integration to use a TLS certificate. If a specific TLs cert is given, it will use that and force TLS. If no cert is configured, it'll fall back to the developer cert.

The one part that's a bit awkward is handling the TrustServerConnection property on connection strings - this attribute is required if the dev cert is older than version 6 as those versions didn't include 127.0.0.1 in the SAN. What I have works, but I'm not entirely happy with it.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
  • Does the change require an update in our Aspire docs?
    • No

@github-actions
Copy link
Contributor

github-actions bot commented Nov 22, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13131

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13131"

@afscrome afscrome requested a review from danegsta November 22, 2025 19:45
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 22, 2025
.WithServerAuthenticationCertificateConfiguration(async ctx =>
{
builder.CreateResourceBuilder((SqlServerServerResource)ctx.Resource)
.WithContainerFiles("/var/opt/mssql/", [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be updating the context on the callback version of WithContainerFiles to receive the server authentication configuration (marked as experimental like the rest of the API to begin with), so this will be able to be expressed as a simple WithContainerFiles instead of needing to chain it inside WithServerAuthenticationCertificateConfiguration since it's possible certificate configuration will need to be just one part of building a config file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #13151

I stuck with using ReferenceExpressions for path values even in the WithContainerFiles callback as I'm taking advantage of the ability to determine whether an particular reference was actually resolved to determine what key formats I actually need to make available for a given resource (and if no resources are actually referencing a private key, there's no reason to export any of the private key material).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this look?

@afscrome
Copy link
Contributor Author

afscrome commented Dec 3, 2025

@danegsta test failure is weird. The error is semi legitimate, but it's weird that it only failed one test and not the whole lot - https://github.com/dotnet/aspire/actions/runs/19837825119/job/56839351366?pr=13131#step:22:291 .

You can get the same error locally if you return an empty array from WithContainerFiles(). This suggest that ctx.ServerAuthenticationCertificateContext was null in one test, but not others which seems odd. I'd have expected it to be null for either all of the tests in a job, or none of the tests..

    .WithContainerFiles("/tmp", async (ctx, ct) =>
    {
        return [];
    });

@danegsta
Copy link
Member

danegsta commented Dec 3, 2025

@danegsta test failure is weird. The error is semi legitimate, but it's weird that it only failed one test and not the whole lot - https://github.com/dotnet/aspire/actions/runs/19837825119/job/56839351366?pr=13131#step:22:291 .

You can get the same error locally if you return an empty array from WithContainerFiles(). This suggest that ctx.ServerAuthenticationCertificateContext was null in one test, but not others which seems odd. I'd have expected it to be null for either all of the tests in a job, or none of the tests..

    .WithContainerFiles("/tmp", async (ctx, ct) =>
    {
        return [];
    });

Yeah, I need to update the handling in Aspire to skip empty results.

@danegsta
Copy link
Member

danegsta commented Dec 4, 2025

@afscrome the empty files behavior should be fixed in main

@danegsta
Copy link
Member

It feels like we should update the connection strings as well to enforce TLS IF SSL is enabled.

- Updated SQL Server resource to configure a TLS cert when one is configured, and remove the TLS validation bypass from the connection string
- Updated Playground app host to include sql with no TLS, and renamed resources to make their purpose clearer
Copilot AI review requested due to automatic review settings March 21, 2026 13:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the SQL Server hosting integration to take advantage of the (experimental) developer certificate / HTTPS certificate plumbing so SQL Server can use a server-auth certificate and conditionally adjust connection string behavior based on whether TLS is enabled.

Changes:

  • Builds SQL Server (ADO.NET) and JDBC connection strings using EndpointReference.GetTlsValue(...) to conditionally include TrustServerCertificate.
  • Enables TLS for the SQL Server endpoint via SubscribeHttpsEndpointsUpdate(...) and generates mssql.conf with the configured cert/key paths.
  • Adds shared X509Certificate2Extensions to support dev-cert version checks.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File Description
src/Aspire.Hosting.SqlServer/SqlServerServerResource.cs Refactors connection string construction and updates JDBC/connection-string docs.
src/Aspire.Hosting.SqlServer/SqlServerBuilderExtensions.cs Enables TLS based on dev-cert metadata and writes mssql.conf using certificate context.
src/Aspire.Hosting.SqlServer/Aspire.Hosting.SqlServer.csproj Links in shared certificate extensions used by the SQL Server integration.
playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/AppHost.cs Updates playground to exercise new certificate behavior and adds a “no TLS” SQL resource.
Comments suppressed due to low confidence (1)

playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/AppHost.cs:17

  • sqlNoTls is created but never used. With warnings treated as errors, this will fail the build; either use it (e.g., add a database/reference) or assign it to a discard (_ = ...).

Comment on lines +106 to +109

if (certAnnotation == null || certAnnotation.UseDeveloperCertificate == true)
{
var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>();
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In run mode, the dev-cert version gate can be skipped when an HttpsCertificateAnnotation exists with UseDeveloperCertificate == null (default behavior). In that case, TLS gets enabled even for older dev certs, and the connection string will omit TrustServerCertificate, which is what this gate is trying to avoid. Consider computing whether a dev cert is in use via UseDeveloperCertificate.GetValueOrDefault(devCertService.UseForHttps) (and Certificate is null) rather than checking only == true.

Suggested change
if (certAnnotation == null || certAnnotation.UseDeveloperCertificate == true)
{
var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>();
var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>();
bool useDeveloperCertificate;
if (certAnnotation is null)
{
// No annotation: fall back to the developer certificate service setting.
useDeveloperCertificate = devCertService.UseForHttps;
}
else if (certAnnotation.Certificate is not null)
{
// A custom certificate is specified; do not treat this as using the developer certificate.
useDeveloperCertificate = false;
}
else
{
// No explicit certificate: compute effective dev-cert usage from the annotation value,
// defaulting to the developer certificate service setting when null.
useDeveloperCertificate = certAnnotation.UseDeveloperCertificate.GetValueOrDefault(devCertService.UseForHttps);
}
if (useDeveloperCertificate)
{

Copilot uses AI. Check for mistakes.
Comment on lines +101 to +103
/// <para>Format: <c>jdbc:sqlserver://{host}:{port}[];trustServerCertificate=true]</c>.</para>
/// <para>User and password credentials are not included in the JDBC connection string.
/// Use the <see cref="UserNameReference"/> and <see cref="PasswordParameter"/> connection properties to access credentials.</para>
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XML docs for JdbcConnectionString have malformed format text ([] / trailing ]) and now point to UserNameReference / PasswordParameter, but the connection properties exposed are named Username and Password (see IResourceWithConnectionString.GetConnectionProperties). Update the docs to reflect the actual JDBC format and reference the correct connection property names.

Suggested change
/// <para>Format: <c>jdbc:sqlserver://{host}:{port}[];trustServerCertificate=true]</c>.</para>
/// <para>User and password credentials are not included in the JDBC connection string.
/// Use the <see cref="UserNameReference"/> and <see cref="PasswordParameter"/> connection properties to access credentials.</para>
/// <para>Format: <c>jdbc:sqlserver://{Host}:{Port}[;databaseName={Database}][;trustServerCertificate=true]</c>.</para>
/// <para>User and password credentials are not included in the JDBC connection string.
/// Use the <c>Username</c> and <c>Password</c> connection properties to access credentials.</para>

Copilot uses AI. Check for mistakes.
Comment on lines +109 to +112
var devCertService = ctx.Services.GetRequiredService<IDeveloperCertificateService>();
var cert = devCertService.Certificates.First();
if (cert.GetCertificateVersion() < 6)
{
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devCertService.Certificates.First() will throw if there are no trusted dev certificates (the service can return an empty list). Guard against an empty certificate list and skip enabling TLS (or fall back to TrustServerCertificate) when no cert is available.

Copilot uses AI. Check for mistakes.
@afscrome
Copy link
Contributor Author

Got this working, although not entirely happy with two things

  1. I'm somewhat lying with endpoint.TlsEnabled, using it to describe whether the TLS cert is trusted, rather than whether TLS is being used or not. And ultimately being used to decide whether we need to include TrustServerCertificate on the connection string
  2. The SubscribeHttpsEndpointsUpdate implementation to ensure that we continue to include the TrustServerCertificate portion of the connection string if the dev cert is not version 6 or higher.

@danegsta
Copy link
Member

I was meaning to follow up with you on this; I merged #14919 recently which adds conditional expression support to reference expressions and should let you simplify the certificate support checks. I'm currently using this to control whether ,ssl=true gets added to the Redis connection string based on whether an endpoint is TLS enabled.

The main requirement to take advantage of this would be to expose a ReferenceExpression property from IDeveloperCertificateService that indicates whether the latest certificate supports loopback IPs as bool.TrueString/bool.FalseString values. Then you can gait a conditional value with something like ReferenceEcpression.CreateConditional(certificateService.SupportsLoopbackIp, bool.TrueString, ReferenceExpression.Create($"whatever is required if true"), ReferenceExpression.Create($"whatever is required if false")).

I typed that up on my phone, so apologies if any of the syntax is off.

@danegsta
Copy link
Member

Main idea of the conditional version of reference expressions is that it takes an IValueProvider as the conditional, a string that'll be compared case insensitively against the value, and two ReferenceExpression branches; the first is used if the values match, the second if they don't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants